-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
follows: refactor FollowButton.js #1780
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks good already, I just left some general questions. Also, should we add some tests?
return ( | ||
<span className="a4-follow"> | ||
<span className={`a4-follow ${useMbStyles ? 'a4-follow--proposal-style' : ''}`}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of asking for specific projects, can we not add like a className
prop or customClassName
or similar and then you pass a4-follow--proposal-style
in MB as a prop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh good catch! that was unintended, i forgot to change it!
const buttonClasses = following ? 'a4-btn a4-btn--following' : 'a4-btn a4-btn--follow' | ||
|
||
const AlertPortal = () => { | ||
if (!alert || !alertTarget) return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this actually be &&
or ||
?
const container = document.getElementById(alertTarget) | ||
if (!container) return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a console.error
or throw an error, I think that's better for devs to understand where the issue is then.
009715a
to
8f018fb
Compare
8f018fb
to
3f25bd4
Compare
Description
Refactor FollowButton to allow for alerts outside the container